-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Allow linking a prebuilt optimized compiler-rt builtins library #143689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Allow linking a prebuilt optimized compiler-rt builtins library #143689
Conversation
This PR modifies If appropriate, please update This PR modifies If appropriate, please update
cc @tgross35 |
This comment has been minimized.
This comment has been minimized.
27fcb13
to
1ea1789
Compare
I don't really know much about the optimized compiler-rt builtins. Maybe @tgross35 might know more? |
Can you say more about how much time this saves? Historically it's been my impression locally that building is basically free, this flag is more about whether it's possible to build (e.g., needs to be off if you're ad-hoc cross compiling). Though maybe I'm remembering wrong? |
The goal is to enable optimized compiler-builtins in fedora packaging. There, rust is built using the distro provided llvm and resulting libraries. Some runtime features are absent if optimized compiler-builtins are not used. Notably, outlined aarch64 LSE atomic operations are not available without this support. |
Right, it's less about time and more about wanting to use our existing system LLVM packages. |
Ping @tgross35. The intent here is to enable optimized-builtins in Fedora using the system's compiler-rt.builtins library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to compiler-builtins lgtm with a possible rename of the var, assuming you have verified this works locally. Somebody more familiar with bootstrap (@Kobzol?) will need to review the changes there.
1ea1789
to
8ce8e15
Compare
I did test builds with the fedora targets (excepting s390x, it lacks a libcompiler-rt.builtins library in rawhide right now). Likewise, I verified the LSE support linked in for aarch64 does work. Is it a realistic case to consider something trying to link libgcc.a or libcompiler-rt.builtins.a when std already includes a copy of libcompiler-rt.builtins? That case I haven't tested. |
if let Some(dir) = rt_builtins_ext.parent() { | ||
println!("cargo::rustc-link-search=native={}", dir.display()); | ||
} | ||
println!( | ||
"cargo::rustc-link-lib=static:+verbatim={}", | ||
rt_builtins_ext.to_str().unwrap() | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does println!("cargo::rustc-link-arg={}", rt_builtins_ext.to_str().unwrap())
work to replace these by chance? I think passing the exact path to the linker should avoid needing to set both -l
and -L
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like it should work, but trying locally (x86-64-unknown-linux-gnu) didn't work as expected. The builtins library wasn't copied into the .rlib
as is the case with the above or letting rust built them.
I think this can happen in niche build setups, but no special handling should be needed; all three libraries make their builtin symbols weak, so conflicts aren't a problem. |
☔ The latest upstream changes (presumably #144773) made this pull request unmergeable. Please resolve the merge conflicts. |
8ce8e15
to
f8de502
Compare
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? `@tgross35`
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ``@tgross35``
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ```@tgross35```
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ````@tgross35````
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? `````@tgross35`````
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ``````@tgross35``````
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ```````@tgross35```````
Rollup merge of #144705 - pmur:murp/aarch64-lse, r=Amanieu compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for #143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ```````@tgross35```````
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang/rust#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ```````@tgross35```````
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang/rust#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ```````@tgross35```````
compiler-builtins: plumb LSE support for aarch64 on linux/gnu when optimized-compiler-builtins not enabled Add dynamic support for aarch64 LSE atomic ops on linux/gnu targets when optimized-compiler-builtins is not enabled. Enabling LSE is the primary motivator for rust-lang#143689, though extending the rust version doesn't seem too farfetched. Are there more details which I have overlooked which make this impractical? I've tested this on an aarch64 host with LSE. r? ```````@tgross35```````
f8de502
to
b382478
Compare
The rust versions are weak, I am not sure the library built by optimized-compiler-builtins or the system provided libclang_rt.builtins.a exposes weak builtin functions. When I tried explicitly linking a second time on aarch64, it raised errors. |
@tgross35 ping, are there any more changes needed for this patch? |
I was just waiting for somebody from bootstrap to +1 the changes there, @rust-lang/bootstrap would anybody mind taking a look? I guess it would probably wouldn't be a bad idea to at least mention this (and the other config) in ## Configuration
`compiler-builtins` can be configured via the following environment variables:
- `LLVM_COMPILER_RT_LIB`
- `RUST_COMPILER_RT_ROOT `
See `build.rs` for details.
## Contributing
... |
b382478
to
1b5dd10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look. Fine with the approach in general, but I'd appreciate two changes:
- It would be more consistent if also the
build.optimized-compiler-builtins
option took a path, so that we don't have the same named option that takes different values inbuild
and in thetarget
config. - It would be nicer to create an enum, e.g.
CompilerBuiltinsConfig
, which would have two variants (Build
andUseExisting(PathBuf)
) and parse this enum from the TOML config files immediately, and then work with it in bootstrap, rather than converting the typed inoptimized_compiler_builtins
. That would also allow us to have only a single function inConfig
, and wouldn't require creatingoptimized_compiler_builtins_path
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I think it's much easier to understand now! I'd appreciate some small comment changes, but other than that LGTM.
This comment has been minimized.
This comment has been minimized.
118d8d0
to
1a868a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this changes config options, you could also add a change entry to change_tracker.rs
, but this is probably so niche that it's not even needed.
LGTM from my side.
1a868a3
to
ad333e4
Compare
Extend the <target>.optimized-compiler-builtins bootstrap option to accept a path to a prebuilt compiler-rt builtins library, and update compiler-builtins to enable optimized builtins without building compiler-rt builtins.
Create a dedicated enum to abstract the different ways compiler-builtins can be configured. This also relaxes build.optimized-compiler-builtins to accept the path of a library to match the behavior of <target>.optimized-compiler-builtins override.
ad333e4
to
12172b6
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Thanks. I also tried to improve the documentation in bootstrap.example.toml too. |
Extend the .optimized-compiler-builtins bootstrap option to accept a path to a prebuilt compiler-rt builtins library, and update compiler-builtins to enable optimized builtins without building compiler-rt builtins.